PYTHON-6074 Fix pool deadlock when a greenlet is killed during checkin - #3041
PYTHON-6074 Fix pool deadlock when a greenlet is killed during checkin#3041blink1073 wants to merge 8 commits into
Conversation
# Conflicts: # doc/changelog.rst
There was a problem hiding this comment.
🟡 Changes recommended
The changelog entry header uses an invalid placeholder date and the noted target version doesn’t match the PR description, so release documentation needs to be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a gevent-specific deadlock in PyMongo’s connection pool that can occur when a greenlet is killed during connection check-in, leaving pool accounting counters permanently inflated and causing subsequent checkouts to block indefinitely.
Changes:
- Makes pool check-in accounting + connection return happen as a single uninterruptible critical section (with re-application if interrupted during lock acquisition under gevent).
- Adds a gevent churn regression test that kills/restarts workers and fails on stalled operations.
- Adds a changelog entry for the fix (but the new section header currently has a placeholder date/version mismatch with the PR description).
File summaries
| File | Description |
|---|---|
| test/test_client.py | Generated sync test mirror including the new gevent deadlock regression test. |
| test/asynchronous/test_client.py | Source test changes adding the gevent deadlock regression test. |
| pymongo/synchronous/pool.py | Generated sync mirror of the pool check-in critical section change. |
| pymongo/asynchronous/pool.py | Source pool change: consolidates check-in accounting under size_cond and handles gevent interruption during lock acquisition. |
| doc/changelog.rst | Adds a changelog entry for the fix (currently with a placeholder date in the 4.19.0 header). |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of correctness/maintenance issues to address (over-broad BaseException handling in the new tests and a PR-description vs changelog-version mismatch).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
doc/changelog.rst:5
- The PR description says the fix is documented in the 4.18.0 changelog, but the diff adds a new 4.19.0 section for the entry. Please confirm the intended release target and either move the entry under the correct version section or update the PR description to match.
Changes in Version 4.19.0 (2026/XX/XX)
--------------------------------------
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies core connection-pool concurrency/accounting behavior under greenlet interruption, which warrants careful human validation beyond automated review.
Review details
Suppressed comments (1)
doc/changelog.rst:5
- The 4.19.0 header uses a non-date placeholder "2026/XX/XX", which isn’t used elsewhere in this changelog and may break any tooling or expectations that this field is a real YYYY/MM/DD date.
Changes in Version 4.19.0 (2026/XX/XX)
--------------------------------------
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Under gevent, notify() is a yield point, so a kill delivered inside it left accounted False and the fallback decremented the accounting a second time. Set the flag before notify() so only interruption during condition acquisition triggers the fallback.
Adds a test interrupting the error handler while it waits to acquire size_cond, covering the fallback that re-applies the checkout accounting.
There was a problem hiding this comment.
🟡 Changes recommended
The new gevent churn tests can leak a global monkeypatch (gevent.thread.sleep) when AMPLIFY_RACE is enabled, which can impact later tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
When AMPLIFY_RACE is enabled the test reassigns gevent.thread.sleep globally; register a cleanup so the widened sleep window doesn't leak into later tests.
There was a problem hiding this comment.
🟡 Changes recommended
The new accounting logic can still miss delivering Condition.notify() if a kill/exception lands during notify(), which can leave waiters blocked indefinitely even though the counters were corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
pymongo/asynchronous/pool.py:1137
- Similar to the checkout error path: if a GreenletExit arrives during _max_connecting_cond.notify() or size_cond.notify(), the connection may already be appended and counters decremented but waiters might never be woken, risking an indefinite hang when there are only waiting checkouts and no further checkins occur. Consider catching BaseException around the notify calls, recording it, re-notifying after reacquiring size_cond (without re-applying accounting), and then re-raising.
accounted = True
if appended:
# Notify any threads waiting to create a connection.
self._max_connecting_cond.notify()
self.size_cond.notify()
doc/changelog.rst:5
- The new 4.19.0 header uses a placeholder date “2026/XX/XX”, which doesn’t match the concrete date format used by other entries in this changelog (e.g., 4.18.0 uses YYYY/MM/DD). Consider using the project’s standard placeholder (if any) or omitting the date until it’s known to keep formatting consistent.
Changes in Version 4.19.0 (2026/XX/XX)
--------------------------------------
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The changelog entry introduces an invalid placeholder date format (2026/XX/XX) that should be replaced with an explicit placeholder (e.g., “TBD”) or a real release date.
Review details
Suppressed comments (1)
doc/changelog.rst:4
- The 4.19.0 changelog header uses a placeholder date (2026/XX/XX), which is inconsistent with the rest of the changelog’s YYYY/MM/DD format and can be confusing for readers. Prefer a real date at release time or an explicit placeholder like “TBD”.
Changes in Version 4.19.0 (2026/XX/XX)
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
PYTHON-6074
Changes in this PR
Under gevent, killing a greenlet that is checking a connection back into the pool can leave the pool's
requestsandactive_socketscounters permanently inflated atmaxPoolSize. Every later checkout then blocks forever on the size-gate wait, freezing all database operations for the process. This change makes the pool's checkin accounting uninterruptible by aGreenletExit, so the counters and the connection are always restored.Pool.checkinapply its counter decrement and connection return in one critical section, re-applying them if aGreenletExitinterrupts before they complete.test_gevent_kill_churn_deadlock, which runs workers and a killing reaper under gevent and fails if operations stall.Test Plan
waitQueueTimeoutMS); the same script runs clean for 5 minutes with the fix.test_gevent_task,test_gevent_timeout,test_gevent_timeout_when_creating_connection) still pass.just lintandjust typingare clean.Checklist
Checklist for Author
Checklist for Reviewer